Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle errors from the oauth/token endpoint #602

Merged
merged 1 commit into from
May 2, 2019
Merged

Handle errors from the oauth/token endpoint #602

merged 1 commit into from
May 2, 2019

Conversation

mdlavin
Copy link
Contributor

@mdlavin mdlavin commented May 1, 2019

Before this change, failures from the OAuth API would result in Invalid JSON received from the Stripe API errors.

The cause was not a parse error, but an error thrown at https://github.com/stripe/stripe-node/compare/master...mdlavin:handle-oauth-errors?expand=1#diff-404ddfdf655bbca4b088b8c7242d0a3aL155 when the headers value was assigned to a string value.

The response body coming back from the oauth/token endpoint is:

{
  "error":"invalid_grant",
  "error_description":"This authorization code has already been used. All tokens issued with this code have been revoked."
}

As described in https://stripe.com/docs/connect/oauth-reference#post-token-errors

I also added a nicely typed Error for invalid_token responses, and the other OAuth failures will be reported as Generic errors instead of Invalid JSON errors

@ob-stripe
Copy link
Contributor

Thanks for the contribution @mdlavin!

r? @rattrayalex-stripe

rattrayalex-stripe
rattrayalex-stripe approved these changes May 1, 2019
View reviewed changes
Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya Matt! Thanks for the contribution!

Does the test fail without the added application code in lib/StripeResource.js? (Pretty sure it would).

Assuming so, this LGTM! (Will await an affirmative response, or verify myself when I have time, before merging).

@mdlavin
Copy link
Contributor Author

mdlavin commented May 1, 2019

Yes, the test fails without the changes in lib/StripeResource.js. I reverted to master locally and here is the failure:

  18 passing (110ms)
  1 failing

  1) StripeResource
       Retry Network Requests
         _request
           should handle OAuth errors gracefully:

      Uncaught AssertionError: expected 'StripeAPIError' to equal 'StripeInvalidGrantError'
      + expected - actual

      -StripeAPIError
      +StripeInvalidGrantError
      
      at /Users/mattlavin/Projects/stripe-node/test/StripeResource.spec.js:244:31
      at Timeout._onTimeout (lib/utils.js:39:295)

The StripeAPIError has the message Invalid JSON received from the Stripe API

@rattrayalex-stripe rattrayalex-stripe merged commit 6fffb29 into stripe:master May 2, 2019
@rattrayalex-stripe
Copy link
Contributor

Awesome, thanks so much Matt!

@mdlavin
Copy link
Contributor Author

mdlavin commented May 3, 2019

@rattrayalex-stripe thanks for merging. Are new releases regularly scheduled?

@mdlavin mdlavin deleted the handle-oauth-errors branch May 3, 2019 14:41
@rattrayalex-stripe
Copy link
Contributor

Released!

We generally try to release ~immediately after merging master; apologies for the delay.

This was referenced Nov 29, 2019
Open
Closed
@snyk-bot snyk-bot mentioned this pull request Dec 15, 2019
Open
This was referenced Apr 2, 2020
Closed
Merged
@snyk-bot snyk-bot mentioned this pull request Apr 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rattrayalex-stripe rattrayalex-stripe rattrayalex-stripe approved these changes

Assignees

@rattrayalex-stripe rattrayalex-stripe

Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mdlavin @ob-stripe @rattrayalex-stripe @stripe-ci